Skip to content

docs(types) mention computed object key syntax and correct existing case #3323

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 21, 2019

Conversation

EugeneHlushko
Copy link
Member

@EugeneHlushko EugeneHlushko commented Oct 20, 2019

  • add the missing <> syntax for computed object props to the writers guide
  • change recently updated example accordingly

Reference of current usage: https://webpack.js.org/configuration/entry-context/#entry

@netlify
Copy link

netlify bot commented Oct 20, 2019

Preview is ready

Built with commit ff14ccf

https://deploy-preview-3323--webpackjsorg-netlify.netlify.com

@opl-
Copy link
Contributor

opl- commented Oct 20, 2019

I would argue that using a TypeScript-like syntax instead of making up our own is a better solution.

First, both JS veterans and newbies alike benefit from it: to the former the syntax might already be well known and intuitive, while for the latter it allows them to get familiar with a syntax they are likely to encounter later on as they learn the language.

Second, TypeScript has already solved the problem of ambiguous types and difficulties in expressing them. Due to the projects nature it had to come up with ways of denoting the types.

Next, I want to look at the current methods of denoting the types to determine problems with them.

In arrays, the usage of a comma to denote different possible types is confusing, as commas are already used to separate values in an array. [string, RegExp] can be interpreted as "string at index 0, regex at index 1", which is not the case according to the current writer's guide. However, in a case where that would be the intent, another problem arises. We cannot use commas to separate the different indexes because the character is already used to separate possible types!

In objects the lack of a separator between the property name and value can also be confusing. Most developers are already used to the concept of the two being separated by : instead of a space. This could also be confusing to new users, who are likely to look for that character as its something they are already familiar with.

Additionally the usage of <> to mark computed property names is once again different from the standard, already existing JavaScript syntax. Computed property names in JS are surrounded in [] and seeing <> instead is inherently confusing to anyone who doesn't know the syntax. <> is not a standard concept in vanilla JS.

Lastly, while using spaces to separate possible types seems like a good idea, it falls apart as soon as we are inside of an array or an object. I'm not exactly opposed to the idea of separating the top level types with spaces, as it can help differentiate between them (even if the difference between the white background and the #f5f7f7 color of the code elements is tiny), but I don't think it's a good idea to use it inside of objects or arrays, and a clear separator character seems like a clearer option there.

@opl-
Copy link
Contributor

opl- commented Oct 20, 2019

Just remembered that you mentioned that having different value types in a single array is not something that has happened, but it's not unheard of. ESLint, for example, uses ['error', {ruleOption: 'value'}] to configure rules.

I also forgot to address default values, but that's because the way they're currently handled seems sane. I don't see a reason to change that.

@montogeek
Copy link
Member

@opl- Yes, I have the same concerns when we decide to do this. Perfect solution would be use the same types notations as webpack source code (if available).

@EugeneHlushko EugeneHlushko merged commit a79228b into master Oct 21, 2019
@EugeneHlushko EugeneHlushko deleted the contrib-types-object branch October 21, 2019 12:30
@EugeneHlushko
Copy link
Member Author

Thanks for reviews guys, looks like we can up an issue to discuss typescript usage and potential impact

@EugeneHlushko
Copy link
Member Author

@opl- @montogeek one of you guys want to take the lead on it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants